@W-20931847 - OMS multiple shipment support with pickups#3613
@W-20931847 - OMS multiple shipment support with pickups#3613sf-deepali-bharmal merged 2 commits intodevelopfrom
Conversation
sf-deepali-bharmal
added
the
skip changelog
| </Box> | ||
| </Stack> | ||
| ) | ||
|
|
There was a problem hiding this comment.
Implementation is same as before, just extracted into a function.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
sf-deepali-bharmal
added
skip changelog
sf-deepali-bharmal
force-pushed
the
W-20931847/refactor
branch
from
05001eb to
64495f6
Compare
| const isOmsMultiShipment = useMemo( | ||
| () => | ||
| isOmsOrder && | ||
| ((order?.omsData?.shipments?.length ?? 0) > 1 || (order?.shipments?.length ?? 0) > 1), |
There was a problem hiding this comment.
This doesnt distinguish between instore and delivery shipments, if we have 1 in store and 1 delivery we'll remove the address, is that ok? It probably is but just want to make sure
There was a problem hiding this comment.
For in store - there is a existing PickupAddress component that will display pickup Addresses separately.
And we will list down all the ShippingMethods from OMS.
There was a problem hiding this comment.
Idea is whenever there is more than one omsData.Shipments or more than one eCom shipmentAddresses , its a multi-shipment scenario -> We dont try to co-relate them irrespective of in-store or online shipments, because we don't have that much data.
PickupAddresses are already shown as is - we dont touch that and thats not a problem because PWA does not show shipping method next to it already, so we are good.
For ShippingMethod and ShippingAddress below it, we show the same pattern like before. In case of multi-shipment don't show addresses.
There was a problem hiding this comment.
From a user’s perspective, the UX feels a bit confusing. We’re showing multiple shipping methods with different statuses (pickup, shipped, etc.), but we’re not showing which order items those methods correspond to. How will users know which items are ready for pickup versus shipped?”
There was a problem hiding this comment.
Here is the existing BOPIS with multi-shipment work.
#3414
Looks like even without OMS PWA KIT inherently does not have the UI to map Shipping info to order line item.
But its a good point. I will look more into better multi shipment mapping in future releases.
But its not something we can cover in current PR.
| const isOmsMultiShipment = useMemo( | ||
| () => | ||
| isOmsOrder && | ||
| ((order?.omsData?.shipments?.length ?? 0) > 1 || (order?.shipments?.length ?? 0) > 1), |
There was a problem hiding this comment.
Why are we checking for order?.shipments?.length?
Isn't the multi shipment check specifically for omsOrders?
I think it's checking for multi shipments in both oms and normal orders array.
Can we rename the variable in that case?
There was a problem hiding this comment.
I can rename it to - isMultiShipmentOrder ?
| isOmsOrder && | ||
| (order.omsData.shipments?.length > 1 || | ||
| order.shipments?.length > 1) | ||
| {!isOmsMultiShipment && |
There was a problem hiding this comment.
It seems like we are checking only for multi shipments here. So, the variable name is confusing.
On lines 408 and below, we are showing shipping method name and status from normal order shipment even it's not from oms.
There was a problem hiding this comment.
We want to check for both oms order and multi-shipment. I renamed variable to make it cleaner.
sf-deepali-bharmal
dismissed stale reviews from ddiazccrz and sf-xingquan-jin
via
38eaede
sf-deepali-bharmal
removed
the
skip changelog
nit Empty commit to trigger CI Empty commit to trigger CI
rename variable
sf-deepali-bharmal
force-pushed
the
W-20931847/refactor
branch
from
38eaede to
04aa883
Compare
5 participants
Description
This PR removes reliability on indexes while traversing through omsShipping info.
Previously, shipping methods were only shown under Shipping Address. That meant OMS data (provider, status, tracking) was missing for BOPIS pickup shipments.
Shipping method is now shown separately from the shipping-address list. As a result, we can surface OMS shipping methods for both delivery and pickup, so BOPIS orders show OMS info for pickups as well.
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization